Skip to content

ast_lowering: Introduce lower_span for catching all spans entering HIR #88208

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 29, 2021
Merged

ast_lowering: Introduce lower_span for catching all spans entering HIR #88208

merged 1 commit into from
Aug 29, 2021
+330 −169

Conversation

petrochenkov
Copy link
Contributor

This PR cherry-picks the fn lower_span change from #84373.
I also introduced fn lower_ident for lowering spans in identifiers, and audited places where HIR structures with spans or identifiers are constructed and added a few missing lower_spans/lower_idents.

Having a hook for spans entering HIR can be useful for things other than #84373, e.g. #35148.
I also want to check whether this change causes perf regressions due to some accidental inlining issues.

r? @cjgillot

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 21, 2021
@petrochenkov
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion.

@rustbot label: +S-waiting-on-perf

@rustbot rustbot added the S-waiting-on-perf Status: Waiting on a perf run to be completed. label Aug 21, 2021
@bors
Copy link
Collaborator

bors commented Aug 21, 2021

⌛ Trying commit a611199277e1ee5f0af204d9d4132ee6c65ce811 with merge 6f99ad894b945a464742e58a6d3cbae7b7a382a6...

@petrochenkov petrochenkov removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 21, 2021
@bors
Copy link
Collaborator

bors commented Aug 21, 2021

☀️ Try build successful - checks-actions
Build commit: 6f99ad894b945a464742e58a6d3cbae7b7a382a6 (6f99ad894b945a464742e58a6d3cbae7b7a382a6)

@rust-timer
Copy link
Collaborator

Queued 6f99ad894b945a464742e58a6d3cbae7b7a382a6 with parent db002a0, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (6f99ad894b945a464742e58a6d3cbae7b7a382a6): comparison url.

Summary: This benchmark run did not return any relevant changes.

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up.

@bors rollup=never
@rustbot label: +S-waiting-on-review -S-waiting-on-perf -perf-regression

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-perf Status: Waiting on a perf run to be completed. labels Aug 21, 2021
@petrochenkov
Copy link
Contributor Author

So this is not a source of regressions in #84373.
@bors r=maybe

@bors
Copy link
Collaborator

bors commented Aug 21, 2021

📌 Commit a611199277e1ee5f0af204d9d4132ee6c65ce811 has been approved by maybe

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 21, 2021
@petrochenkov
Copy link
Contributor Author

@bors r-
@bors rollup=maybe

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 21, 2021
@petrochenkov petrochenkov added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 21, 2021
@petrochenkov petrochenkov mentioned this pull request Aug 21, 2021
Merged
@Aaron1011
Copy link
Contributor

It seems like it would be very easy to forget to call lower_span somewhere, since the input and output types are both Span. What will happen to relative span encoding if there's an 'unlowered' Span?

@petrochenkov
Copy link
Contributor Author

@Aaron1011

What will happen to relative span encoding if there's an 'unlowered' Span?

Not setting the span's DefId parent means giving up on the specific incremental optimization introduced by #84373, but otherwise it doesn't affect correctness.
Spans decoded from other crates don't get parent DefIds at all, for example.

@cjgillot
Copy link
Contributor

Normally, we would only have a spurious red DepNode and query recomputation.

@cjgillot
Copy link
Contributor

I am not really the most neutral reviewer as I essentially wrote the same code. OTOH, this PR is harmless, up to making LLVM chew more code. @Aaron1011 do you have any concern about merging this?

@inquisitivecrystal inquisitivecrystal added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Aug 24, 2021
@bors

This comment has been minimized.

Sign in to view
@petrochenkov
Copy link
Contributor Author

r? @Aaron1011

@rust-highfive rust-highfive assigned Aaron1011 and unassigned cjgillot Aug 25, 2021
@bors

This comment has been minimized.

Sign in to view
@Aaron1011
Copy link
Contributor

r=me with the merge conflicts fixed

@petrochenkov
Copy link
Contributor Author

@bors r=Aaron1011

@bors
Copy link
Collaborator

bors commented Aug 29, 2021

📌 Commit 59013cd has been approved by Aaron1011

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 29, 2021
@bors
Copy link
Collaborator

bors commented Aug 29, 2021

⌛ Testing commit 59013cd with merge ef52471...

@bors
Copy link
Collaborator

bors commented Aug 29, 2021

☀️ Test successful - checks-actions
Approved by: Aaron1011
Pushing ef52471 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 29, 2021
@bors bors merged commit ef52471 into rust-lang:master Aug 29, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 29, 2021
This was referenced Aug 29, 2021
Closed
Merged
Merged
Merged
Merged
@petrochenkov petrochenkov deleted the lowspan branch February 22, 2025 18:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@Aaron1011 Aaron1011

Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.56.0
Development

Successfully merging this pull request may close these issues.
8 participants
@petrochenkov @rust-timer @bors @Aaron1011 @cjgillot @rust-highfive @inquisitivecrystal @rustbot